Introduction of `SamplingContext` #253

torfjelde · 2021-05-31T23:33:43Z

This PR introduces the outer-most approach to SamplingContext as discussed in #249, essentially an adaption of https://github.com/TuringLang/DynamicPPL.jl/tree/dw/samplingcontext to fit the tilde-simplification in #252.

To discuss

Should we rename DefaultContext to JointContext?
- I left it as is because I was uncertain if this cause down-stream issues, and thus whether we should maybe wait with this?

src/varname.jl

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

torfjelde · 2021-05-31T23:45:09Z

src/model.jl


 Evaluate the `model` with the arguments matching the given `sampler` and `varinfo` object.
 """
 @generated function _evaluate(
-    rng, model::Model{_F,argnames}, varinfo, sampler, context
+    model::Model{_F,argnames}, varinfo, context
 ) where {_F,argnames}
    unwrap_args = [:($matchingvalue(sampler, varinfo, model.args.$var)) for var in argnames]


@devmotion So this won't work anymore since sampler is no longer available. It also seems to me that this didn't work in your PR, right?

I made an attempt at "solving" it.

Yeah the PR was not complete, sorry. I really just wanted to quickly sketch the approach 😬

Wouldn't it be more natural to handle this in a new matchingvalue(context, varinfo, var) where one could define

function matchingvalue(context::SamplingContext, varinfo, var) return matchingvalue(context.sampler, varinfo, var) end

(such that nothing is broken for SamplingContext) and possibly

function matchingvalue(context::AbstractContext, varinfo, var) return matchingvalue(SampleFromPrior(), varinfo, var) end

(is this useful at all?) or

matchingvalue(context::AbstractContext, varinfo, var) = var

(probably more natural).

Sure, that makes sense 👍

src/compiler.jl

src/context_implementations.jl

src/compiler.jl

devmotion · 2021-06-01T07:57:18Z

src/context_implementations.jl

+Falls back to
+```julia
+tilde_assume(context.rng, context.context, context.sampler, right, vn, inds, vi)
+```


Maybe we should not do this and only fall back to assume(rng, ...) in tilde_assume(::SamplingContext{<:DefaultContext}, ...) etc.? I.e. in the child_of_c === nothing branch, we could throw a MethodError and even mention the context c for which no method exists.

A bit confused about what you mean. Do you mean that this would replace the tilde_assume(rng, ...) implementation for DefaultContext (and similarly for the others)?

I mean: Maybe we should not decompose SamplingContext at this part of the pipeline but only throw an error if there is no child context (the other branch where SamplingContext is propagated downwards is fine). Then

tilde_assume would always have a consistent function signature tilde_assume(context, right, vn, inds, vi)

we would define tilde_assume(::SamplingContext{<:LikelihoodContext}, right, ...) etc. instead of tilde_assume(rng, ....)

decompose SamplingContext in the definition of tilde_assume(::SamplingContext{<:LikelihoodContext}, ...) etc. and call assume(rng, ...)

The motivation would be that the tilde pipeline is "cleaner"/more consistent (possibly) but still SamplingContext would be decomposed for everyone that implements assume for a sampler.

Cool, that's what I thought (and meant by "would replace the tilde_assume(rng, ...) implementation for DefaultContext").

And yeah, I guess that seems like an improvement over the current.

I still don't like that we have to implement both tilde(::SamplingContext{<:PrimitiveContext}, ...) and tilde(::PrimitiveContext, ...) 😕 Btw, have you had a look at the new WrappedContext PR? Maybe the benefits are a bit clearer now, in particular the implementation of this part is IMO much simpler than this PR. (Yeaaah sorry, still can't quite let it go 🙃 )

Actually, what do we do with contexts such as LikelihoodContext? Currently we perform this setting of variables both in the assume(...) and assume(rng, ...). If we drop the assume(rng, ...) statement in favour of assume(::SamplingContext{<:LikelihoodContext}, ...) we need to reconstruct SamplingContext{<:LikelihoodContext} in this, no?

E.g.

function tilde_assume( rng::Random.AbstractRNG, context::LikelihoodContext{<:NamedTuple}, sampler, right, vn, inds, vi, ) if haskey(context.vars, getsym(vn)) vi[vn] = vectorize(right, _getindex(getfield(context.vars, getsym(vn)), inds)) settrans!(vi, false, vn) end return tilde_assume(rng, LikelihoodContext(), sampler, right, vn, inds, vi) end

has to become

function tilde_assume( context::SamplingContext{<:LikelihoodContext{<:NamedTuple}}, right, vn, inds, vi, ) child_context = context.context if haskey(child_context.vars, getsym(vn)) vi[vn] = vectorize(right, _getindex(getfield(child_context.vars, getsym(vn)), inds)) settrans!(vi, false, vn) end # Rewrap in `SamplingContext.` new_context = SamplingContext(context.rng, context.sampler, LikelihoodContext()) return tilde_assume(new_context, right, vn, inds, vi) end

Similarly for dot_tilde_assume, and for PriorContext, etc.

This seems quite, hmm, annoying 😕

Yeah, it seems a bit annoying but I think it's not too bad:

it's just two more lines

I guess you could move the extraction + rewrapping logic to a separate function that could be used by the different contexts and for dot_tilde_assume as well

it will change anyway if we introduce a dedicated context for fixing variables to certain values

it's just two more lines

Sure, I don't mind an additional two lines, but t's just confusing to read IMO. And this is just one example. There are more already, e.g. PriorContext, and there will be more in the future.

I guess you could move the extraction + rewrapping logic to a separate function that could be used by the different contexts and for dot_tilde_assume as well

Yeah, maybe this could be done.

it will change anyway if we introduce a dedicated context for fixing variables to certain values

Will have the same issue with ConditionContext though, but yes, it will decrease the number of contexts which suffer from this from 2 to 1.

(With PrimitiveContext and WrappedContext this becomes a non-issue, just sayin' 🙃 )

src/context_implementations.jl

devmotion · 2021-06-01T08:10:07Z

src/model.jl

+end
+
+(model::Model)(context::AbstractContext) = model(VarInfo(), context)
+function (model::Model)(varinfo::AbstractVarInfo, context::AbstractContext)


Maybe one could be consistent with the other calls in the model and the tilde definitions and make context the first argument (even though it is inconsistent with the current (model::Model)(...) API)?

I'll leave that decision to you. To me it's whatever. I decided not to because I just wanted to try to make as few changes as possible and thus figured it was best to preserve the current ordering of arguments, but as I said, I'm fine with whatever.

One thing to note is that we might at some point want to consider changing the order of the tilde-statements too when we introduce different VarInfo, as we then want to dispatch on the type of vi. With the current signatures, this will inevitably lead to method ambiguities. E.g. maybe we change

tilde_assume(context::SamplingContext, right, vn, inds, vi)

to

tilde_assume(context::SamplingContext, vi::AbstractVarInfo, right, vn, inds)

Granted, if we decide to do this your argument is even stronger:)

devmotion · 2021-06-01T08:16:14Z

src/model.jl


 Evaluate the `model` with the arguments matching the given `sampler` and `varinfo` object.
 """
 @generated function _evaluate(
-    rng, model::Model{_F,argnames}, varinfo, sampler, context
+    model::Model{_F,argnames}, varinfo, context
 ) where {_F,argnames}
    unwrap_args = [:($matchingvalue(sampler, varinfo, model.args.$var)) for var in argnames]


Yeah the PR was not complete, sorry. I really just wanted to quickly sketch the approach 😬

Wouldn't it be more natural to handle this in a new matchingvalue(context, varinfo, var) where one could define

function matchingvalue(context::SamplingContext, varinfo, var) return matchingvalue(context.sampler, varinfo, var) end

(such that nothing is broken for SamplingContext) and possibly

function matchingvalue(context::AbstractContext, varinfo, var) return matchingvalue(SampleFromPrior(), varinfo, var) end

(is this useful at all?) or

matchingvalue(context::AbstractContext, varinfo, var) = var

(probably more natural).

devmotion · 2021-06-01T08:16:52Z

src/varname.jl

@@ -39,3 +39,6 @@ Possibly existing indices of `varname` are neglected.
 ) where {s,missings,_F,_a,_T}
    return s in missings
 end
+
+# HACK: Type-piracy. Is this really the way to go?


No 😄

Where is it needed?

It's just convenient for the dot_tilde_* statements where we are working with vns::AbstractVector{<:VarName{sym}}. This way we can just use getsym as before instead of dispatching on this (which will inevitably lead to method ambiguity due to the ordering of args in tilde)

yebai · 2021-06-01T10:01:47Z

Should we rename DefaultContext to JointContext?

I think it's ok to rename DefaultContext but keep its contractors as depreciated functions for a few weeks. We can remove it completely once all places downstream are changed. Also, maybe use a more descriptive new name, e.g. LogDensityContext. LogDensityContext would be consistent with new APIs sketched in AbstractPPL.

devmotion · 2021-06-01T10:17:04Z

I think it's ok to rename DefaultContext but keep its contractors as depreciated functions for a few weeks. We can remove it completely once all places downstream are changed.

You can use Base.@deprecate_binding to deprecate the type correctly and avoid breaking dispatches in other packages (if the type parameters are still the same).

Co-authored-by: David Widmann <[email protected]>

…xt wrappers

torfjelde · 2021-06-04T07:41:01Z

I made the change I suggested in the previous comment so you can see what it'll look like. You can see the changes here: f52550f

torfjelde · 2021-06-04T23:25:49Z

Thoughts @devmotion ?

torfjelde · 2021-06-05T00:11:51Z

Btw, here's an example of how the different approaches looks for a sampler that overloads tilde_assume: ESS in Turing.jl.

Currently the implementation is https://github.com/TuringLang/Turing.jl/blob/c79dce028b3edd0be284bf28b13d83956606b786/src/inference/ess.jl#L138-L144:

function DynamicPPL.tilde(rng, ctx::DefaultContext, sampler::Sampler{<:ESS}, right, vn::VarName, inds, vi)
    if inspace(vn, sampler)
        return DynamicPPL.tilde(rng, LikelihoodContext(), SampleFromPrior(), right, vn, inds, vi)
    else
        return DynamicPPL.tilde(rng, ctx, SampleFromPrior(), right, vn, inds, vi)
    end
end

With the original suggestion (no EvaluationContext, SamplingContext is outermost), this ends up being:

function DynamicPPL.tilde_assume(context::SamplingContext{<:Sampler{<:ESS}, <:DefaultContext}, right, vn::VarName, inds, vi)
    if inspace(vn, context.sampler)
        return DynamicPPL.tilde_assume(LikelihoodContext(), right, vn, inds, vi)
    else
        return DynamicPPL.tilde_assume(SamplingContext(context.rng, SampleFromPrior()), right, vn, inds, vi)
    end
end

After introduction of EvaluationContext, it remaines the same up-to replacement of DefaultContext with Nothing:

function DynamicPPL.tilde_assume(context::SamplingContext{<:Sampler{<:ESS}, Nothing}, right, vn::VarName, inds, vi)
    if inspace(vn, context.sampler)
        return DynamicPPL.tilde_assume(LikelihoodContext(), right, vn, inds, vi)
    else
        return DynamicPPL.tilde_assume(SamplingContext(context.rng, SampleFromPrior()), right, vn, inds, vi)
    end
end

It's a bit annoying to have to dispatch on this, but it could be replaced with a trait-like thing to indicate whether it's a leaf context or not:

function DynamicPPL.tilde_assume(::IsLeaf, context::SamplingContext{<:Sampler{<:ESS}}, right, vn::VarName, inds, vi)
    if inspace(vn, context.sampler)
        return DynamicPPL.tilde_assume(LikelihoodContext(), right, vn, inds, vi)
    else
        return DynamicPPL.tilde_assume(SamplingContext(context.rng, SampleFromPrior()), right, vn, inds, vi)
    end
end

This is also a bit annoying since all implementers of contexts then need to be aware of this additional detail.

In the inner-most approach for "leaf-contexts", e.g. SamplingContext and EvaluationContext, it looks like

function DynamicPPL.tilde_assume(context::SamplingContext{<:Sampler{<:ESS}}, right, vn::VarName, inds, vi)
    if inspace(vn, context.sampler)
        return DynamicPPL.tilde_assume(LikelihoodContext(), right, vn, inds, vi)
    else
        return DynamicPPL.tilde_assume(SamplingContext(context.rng, SampleFromPrior()), right, vn, inds, vi)
    end
end

i.e. the dispatch is slightly simpler.

NOTE: LikelihoodContext have different impls in the above examples, but they all have a default constructor which results in the same behavior in tilde_assume, e.g. LikelihoodContext() = LikelihoodContext(EvaluationContext()) in the last two examples.

torfjelde · 2021-06-07T07:39:06Z

Sorry to ping you again here, but thoughts on the introduction of EvaluationContext and unwrapping SamplingContext until there's nothing to unwrap anymore @devmotion ?

devmotion · 2021-06-07T08:40:26Z

I thought EvaluationContext is only part of the draft with WrappedContext which we postponed in the meeting and didn't want to pursue initially? And what exactly do you mean by unwrapping SamplingContext until there's nothing to unwrap anymore?

I thought in the meeting we agreed to initially just make a simple change that introduces SamplingContext that wraps the RNG, sampler and context that are currently provided to the model to start transitioning to a AbstractContext-based API but unwrap it again internally such that the tilde pipelines are not affected (apart from the highest level tilde_assume! etc. inside of the model). Is this not the plan anymore?

torfjelde · 2021-06-07T09:19:59Z

I thought EvaluationContext is only part of the draft with WrappedContext which we postponed in the meeting and didn't want to pursue initially? And what exactly do you mean by unwrapping SamplingContext until there's nothing to unwrap anymore?

If you see my 4th and 5th paragraph in #253 (comment) + this f52550f commit shows the changes for that particular introduction.

I thought in the meeting we agreed to initially just make a simple change that introduces SamplingContext that wraps the RNG, sampler and context that are currently provided to the model to start transitioning to a AbstractContext-based API but unwrap it again internally such that the tilde pipelines are not affected (apart from the highest level tilde_assume! etc. inside of the model). Is this not the plan anymore?

If this is the case, I genuinely misunderstood 😕

I thought we were deciding that we should go for the outermost approach with SamplingContext and not the innermost approach, but we're still making a separation between sampling and evaluation (though in your original proposal that was just done implicitly by saying "if you never wrap in SamplingContext it's evaluation").

torfjelde · 2021-06-07T10:17:40Z

I'll make another PR with the additional changes reverted, so that the only introduction is a SamplingContext that is immediately unwrapped in tilde_assume to call tilde_assume(rng, ctx, sampler) as you suggested @devmotion.

phipsgabler · 2021-06-07T10:27:04Z

@torfjelde I made a short summary of my understanding of the design in reflection of what I thought of for the APPL design: https://cryptpad.piratenpartei.de/code/#/2/code/edit/yP0qvsI3vGp4IdWGHffpUwly/. That's more of a very high level "did I get this right, and what overlaps are there?" thing.

This is #253 but the only motivation here is to get `SamplingContext` in, nothing relating to interactions with other contexts, etc. Co-authored-by: Hong Ge <[email protected]>

torfjelde · 2021-06-08T19:27:07Z

I had a look at what you wrote @phipsgabler and yeah that seems right:) Are we having a meeting on Thursday? It might be easier to discuss then.

This is #253 but the only motivation here is to get `SamplingContext` in, nothing relating to interactions with other contexts, etc. Co-authored-by: Hong Ge <[email protected]>

phipsgabler · 2021-06-09T08:13:23Z

Great @torfjelde. Yeah, Hong should have send you an invite?

torfjelde · 2021-06-09T08:22:01Z

He did yes:)

This is #253 but the only motivation here is to get `SamplingContext` in, nothing relating to interactions with other contexts, etc. Co-authored-by: Hong Ge <[email protected]>

torfjelde · 2021-06-10T11:59:59Z

Uuuu why did bors kill this PR? It's probably not making it's way into master at this point, but would be nice to keep the branch around since this is at least going in the right direction. It's no bigge since I have it locally, but just curious what happened here.

yebai · 2021-06-10T12:06:02Z

Looks like a bors bug.

devmotion · 2021-06-10T12:07:50Z

Strange. Did you cherry-pick some commits or rebased the other PR on this one? Or linked the two PRs in some way (but then e.g. Github and not bors should have closed it...)?

torfjelde · 2021-06-10T12:11:51Z

I think what I did was:

Create new branch based on this.
Reset to the commit I wanted.
Work from there.

* initial stuff * moved benchmark folder and added README * unwrap distributions and varnames at model-level * removed _tilde and renamed tilde_assume and others * formatting * updated compiler for new tilde-methods * fixed calls to dot_assume * added sampling context and unwrap_childcontext * updated tilde methods * updated model call signature * updated compiler * formatting * added getsym for vectors * Update src/varname.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed some signatures for Model * fixed a method call * fixed method signatures * sort of fixed the matchingvalue functionality for model * formatting * removed redundant _tilde method * removed left-over acclogp! that should not be here anymore * export SamplingContext * use context instead of ctx to refer to contexts * formatting * use context instead of ctx for variables * use context instead of ctx to refer to contexts * Update src/compiler.jl Co-authored-by: David Widmann <[email protected]> * Update src/context_implementations.jl Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * added some whitespace to some docstrings * deprecated tilde and dot_tilde plus exported new versions * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * minor version bump * added impl of matchingvalue for contexts * reverted the change that makes assume always resample * removed the inds arguments from assume and dot_assume to stay non-breaking * Update src/context_implementations.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * added missing sampler arg to tilde_observe * added missing sampler argument in dot_tilde_observe * fixed order of arguments in some dot_assume calls * formatting * formatting * added missing sampler argument in tilde_observe for SamplingContext * added missing word in a docstring * updated submodel macro * removed unwrap_childcontext and related since its not needed for this PR * updated submodel macro * fixed evaluation implementations of dot_assume * updated pointwise_loglikelihoods and related * added proper tests for pointwise_loglikelihoods * updated DPPL tests to reflect recent changes * bump minor version since this will be breaking * formatting * formatting * renamed mean_of_mean_models used in tests * bumped dppl version in integration tests * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * fixed ambiguity error * Introduction of `SamplingContext`: keeping it simple (#259) This is #253 but the only motivation here is to get `SamplingContext` in, nothing relating to interactions with other contexts, etc. Co-authored-by: Hong Ge <[email protected]> * Update src/DynamicPPL.jl Co-authored-by: David Widmann <[email protected]> * added initial impl of SimpleVarInfo * remove unnecessary debug statements to be compat with Zygote * make reconstruct slightly more generic * added a couple of convenience constructors * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * small fix * return var_info from tilde-statements, allowing impl of immutable versions * allow usage of non-Ref types in SimpleVarInfo * update submodel-macro * formatting and docstring for submodel-macro * attempt at supporting implicit returns too * added a small comment * simplifed submodel macro a bit * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed typo * use bang-bang convention * updated PointwiseLikelihoodContext * fixed issue where we unnecessarily replace the return-statement * check subtype in the retval * formatting * fixed type-instability in retval check * introduced evaluate method for model * remove unnecessary type-requirement * make return-value check much nicer * removed redundant creation of anonymous function * dont use UnionAll in return_values * updated tests for submodel to reflect new syntax * moved to using BangBang-convention for most methods * remove SimpleVarInfo from this branch * added a comment * reverted submodel macro to use = rather than ~ * updated SimpleVarInfo impl * added a couple of missing deprecations * updated tests * updated implementations of logjoint and others * formatting * added eltype impl for SimpleVarInfo * formatting * fixed eltype for SimpleVarInfo * updated to work with master * changed the output structure a bit * forgot to include src * updated jmd files * added some docs * updated README * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: David Widmann <[email protected]> Co-authored-by: Hong Ge <[email protected]>

Cf. [this discussion](#253 (comment)). Also, derive deprecated names automatically. In the next breaking release we could completely get rid of `DEPRECATED_INTERNALNAMES`.

@phipsgabler

* fixed some signatures for Model * fixed a method call * fixed method signatures * sort of fixed the matchingvalue functionality for model * formatting * removed redundant _tilde method * removed left-over acclogp! that should not be here anymore * export SamplingContext * use context instead of ctx to refer to contexts * formatting * use context instead of ctx for variables * use context instead of ctx to refer to contexts * Update src/compiler.jl Co-authored-by: David Widmann <[email protected]> * Update src/context_implementations.jl Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * added some whitespace to some docstrings * deprecated tilde and dot_tilde plus exported new versions * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * minor version bump * added impl of matchingvalue for contexts * reverted the change that makes assume always resample * removed the inds arguments from assume and dot_assume to stay non-breaking * Update src/context_implementations.jl Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * added missing sampler arg to tilde_observe * added missing sampler argument in dot_tilde_observe * fixed order of arguments in some dot_assume calls * formatting * formatting * added missing sampler argument in tilde_observe for SamplingContext * added missing word in a docstring * updated submodel macro * removed unwrap_childcontext and related since its not needed for this PR * updated submodel macro * fixed evaluation implementations of dot_assume * updated pointwise_loglikelihoods and related * added proper tests for pointwise_loglikelihoods * updated DPPL tests to reflect recent changes * bump minor version since this will be breaking * formatting * formatting * renamed mean_of_mean_models used in tests * bumped dppl version in integration tests * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * fixed ambiguity error * Introduction of `SamplingContext`: keeping it simple (#259) This is #253 but the only motivation here is to get `SamplingContext` in, nothing relating to interactions with other contexts, etc. Co-authored-by: Hong Ge <[email protected]> * Update src/DynamicPPL.jl Co-authored-by: David Widmann <[email protected]> * added initial impl of SimpleVarInfo * remove unnecessary debug statements to be compat with Zygote * make reconstruct slightly more generic * added a couple of convenience constructors * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * small fix * return var_info from tilde-statements, allowing impl of immutable versions * allow usage of non-Ref types in SimpleVarInfo * update submodel-macro * formatting and docstring for submodel-macro * attempt at supporting implicit returns too * added a small comment * simplifed submodel macro a bit * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * fixed typo * use bang-bang convention * updated PointwiseLikelihoodContext * fixed issue where we unnecessarily replace the return-statement * check subtype in the retval * formatting * fixed type-instability in retval check * introduced evaluate method for model * remove unnecessary type-requirement * make return-value check much nicer * removed redundant creation of anonymous function * dont use UnionAll in return_values * updated tests for submodel to reflect new syntax * moved to using BangBang-convention for most methods * remove SimpleVarInfo from this branch * added a comment * reverted submodel macro to use = rather than ~ * updated SimpleVarInfo impl * added a couple of missing deprecations * updated tests * updated implementations of logjoint and others * formatting * added eltype impl for SimpleVarInfo * formatting * fixed eltype for SimpleVarInfo * implement setindex!! in prep for allowing sampling with immutable vi * formatting * initial work on allowing sampling using SimpleVarInfo * formatting Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * add constructor for SimpleVarInfo using model * improved leftover to_namedtuple_expr, fixing a bug when used with Zygote * bumped patch version * fixed set_flag!! * forgot the return in the replace_returns * bigboy update to benchmarks * fixed some issues and added support for usage of Dict in SimpleVarInfo * added docstring and improved indexing behvaior for SimpleVarInfo * formatting * dont allow sampling with indexing when using SimpleVarInfo with NamedTuple unless shapes are specified * _setval_kernel and others are only supported by VarInfo atm * fixed typo in comment * added more values_as impls * removed redundant values_from_metadata * fixed bug in push!! for SimpleVarInfo * forgot which branch Im on * added handling of short defs in replace_returns and more docstrings * fixed bug in generate_tilde introduced in a merge * fixed a bug in isfuncdef * fixed tests * formatting * uncomment mistakenly commented code * bumped version * updated doctests * dont carry over bang-bang versions that we dont need for general varinfos * Apply suggestions from @phipsgabler Co-authored-by: Philipp Gabler <[email protected]> * updated tests * removed unnecessary BangBang methods * fixed zygote rule for dot_observe * fixed Setfield.jl + returning VarInfo bug in model-macro * updated tests * fixed docs * formatting * fixed issues when using ThreadSafeVarInfo * fixed _pointwise_observe for ThreadSafeVarInfo * updated ThreadSafeVarInfo * made SimpleVarInfo compat with ThreadSafeVarInfo and added show * added some tests for return-values of models * formatting * fixed doctest for SimpleVarInfo * formatting * removed comparison of show from doctest for SimpleVarInfo * Update src/compiler.jl Co-authored-by: David Widmann <[email protected]> * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * removed OrderedCollections from docs * some additional fixes * fixed method ambiguity and some ill-defined map * renamed evaluate to evaluate!! * added implementations of haskey, getindex and setindex!! for SimpleVarInfo * formatting * dropped redundant definition * use getproperty instead of getindex * fixed method-ambiguity and added some comments * fixed docstring of SimpleVarInfo * fixed docstrings * fixed Project.toml for docs * fixed docstring of canview * fixed docstrings * another attempt at fixing docstrings * added a TODO comment * remove some output from docstring of SimpleVarInfo * fixed haskey and hasvalue for AbstractDict * updated some comments * updated some errors * added sampling dot_assume for SimpleVarInfo * added true versions of density computations to TestUtils * added tests specific for SimpleVarInfo * also document TestUtils * added TestUtils to docs * fixed setindex!! for SimpleVarInfo using AbstractDict * added more tests * formatting * dont use BangBang for setall! * revert unnecessary changes to settrans! * revert unnecessary changes to set_flag! * revert some changes to docstrings * fixed some comments and docstrings * added more convenient logjoint, logprior, and loglikelihood methods * removed unnecessary export * fixed export * use the Setfield impl of getindex, etc. as default and specialize on AbstractDict * fixed docstrings of logjoint, etc. * Apply suggestions from code review Co-authored-by: Philipp Gabler <[email protected]> * fixed docstring for model * replaced return_values by capturing return-value from tilde-statements instead * added some tests for return-value of model * added broadcast_foreach * Apply suggestions from @devmotion Co-authored-by: David Widmann <[email protected]> * remove broadcast_foreach for now * some fixes to ThreadSafeVarInfo * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * fixed docstrings * forgot qualification for set * formatting * added comment about why we cant use MacroTools.isdef * remove unnecessary deprecation * udpated some docstrings * fixed more docstrings * make overloads of BangBang methods qualified * remove overloading of values and instead use values_as without the type specified * Apply suggestions from code review Co-authored-by: David Widmann <[email protected]> * renamed hasvalue for SimpleVarInfo to _haskey * revert changes from previous commit * minor version bump * fixed sampling with ThreadSafeVarInfo * fixed setindex!! for ThreadSafeVarInfo * fixed eltype for ThreadSafeVarInfo wrapping a SimpleVarInfo * fixed a test * relax atol in serialization tests a bit * temporarily disable Julia 1.3 * relax atol for a prior check * Improvements to `@submodel` in #309 (#348) * added prefix keyword argument to submodel-macro * Apply suggestions from code review Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> * converted example in docs into test * fixed docstring * Apply suggestions from code review Co-authored-by: Philipp Gabler <[email protected]> * removed redundant prefix_submodel_context def and added another example to docstring * fixed doctests * attempt at fixing doctests * another attempt at fixing doctests * had a typo in docstring Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Philipp Gabler <[email protected]> * fixed a test case using submodel * improved docstring according to comments by @devmotion Co-authored-by: David Widmann <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Hong Ge <[email protected]> Co-authored-by: Philipp Gabler <[email protected]>

torfjelde added 6 commits June 1, 2021 00:21

added sampling context and unwrap_childcontext

d70e1be

updated tilde methods

f743990

updated model call signature

3d2e7e2

updated compiler

4f1d396

formatting

b187d74

added getsym for vectors

ee99f8c

github-actions bot reviewed May 31, 2021

View reviewed changes

src/varname.jl Outdated Show resolved Hide resolved

torfjelde and others added 2 commits June 1, 2021 00:35

Update src/varname.jl

c4845d0

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

fixed some signatures for Model

a0c05f3

torfjelde commented May 31, 2021

View reviewed changes

torfjelde added 2 commits June 1, 2021 00:49

fixed a method call

307cd7e

fixed method signatures

5972771

github-actions bot reviewed Jun 1, 2021

View reviewed changes

src/compiler.jl Outdated Show resolved Hide resolved

src/compiler.jl Outdated Show resolved Hide resolved

torfjelde added 8 commits June 1, 2021 01:08

sort of fixed the matchingvalue functionality for model

c4ecd0e

formatting

a34b51c

Merge branch 'tor/tilde-simplification' into tor/sampler-context

6368282

removed left-over acclogp! that should not be here anymore

e4a2cf8

export SamplingContext

7605785

use context instead of ctx to refer to contexts

354ac52

formatting

b7a2b3b

use context instead of ctx for variables

9e0fc9a

github-actions bot reviewed Jun 1, 2021

View reviewed changes

src/context_implementations.jl Outdated Show resolved Hide resolved

use context instead of ctx to refer to contexts

7a4a1a3

This was referenced Jun 1, 2021

WrappedContext post introduction of SamplingContext #254

Closed

WrappedContext and separation between sampling and evaluation #249

Closed

devmotion reviewed Jun 1, 2021

View reviewed changes

torfjelde and others added 2 commits June 2, 2021 01:20

Update src/compiler.jl

7899473

Co-authored-by: David Widmann <[email protected]>

Update src/context_implementations.jl

1630476

Co-authored-by: David Widmann <[email protected]>

introduce EvaluationContext and make PriorContext and LikelihoodConte…

f52550f

…xt wrappers

torfjelde changed the base branch from master to tor/tilde-simplification June 4, 2021 07:33

Merge branch 'tor/tilde-simplification' into tor/sampler-context

800ee2d

torfjelde mentioned this pull request Jun 7, 2021

[Merged by Bors] - Introduction of SamplingContext: keeping it simple #259

Closed

bors bot closed this Jun 9, 2021

bors bot deleted the branch tor/tilde-simplification June 9, 2021 21:58

phipsgabler mentioned this pull request Aug 23, 2021

[Merged by Bors] - Remove __rng__ and __sampler__ from INTERNALNAMES #312

Closed

yebai deleted the tor/sampler-context branch January 31, 2022 20:01

Introduction of SamplingContext #253

Introduction of SamplingContext #253

Uh oh!

Conversation

torfjelde commented May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

To discuss

Uh oh!

Uh oh!

torfjelde May 31, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde Jun 1, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

torfjelde Jun 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yebai commented Jun 1, 2021

Uh oh!

devmotion commented Jun 1, 2021

Uh oh!

torfjelde commented Jun 4, 2021

Uh oh!

torfjelde commented Jun 4, 2021

Uh oh!

torfjelde commented Jun 5, 2021

Uh oh!

torfjelde commented Jun 7, 2021

Uh oh!

devmotion commented Jun 7, 2021

Uh oh!

torfjelde commented Jun 7, 2021

Uh oh!

torfjelde commented Jun 7, 2021

Uh oh!

phipsgabler commented Jun 7, 2021

Uh oh!

torfjelde commented Jun 8, 2021

Uh oh!

Introduction of `SamplingContext` #253

Introduction of `SamplingContext` #253

torfjelde commented May 31, 2021 •

edited

Loading

torfjelde May 31, 2021 •

edited

Loading

torfjelde Jun 1, 2021 •

edited

Loading

torfjelde Jun 2, 2021 •

edited

Loading

torfjelde Jun 2, 2021 •

edited

Loading